-
Notifications
You must be signed in to change notification settings - Fork 36
InitContext
, part 4 - Use init!!
to replace evaluate_and_sample!!
, predict
, returned
, and initialize_values
#984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: py/init-prior-uniform
Are you sure you want to change the base?
Conversation
Benchmark Report for Commit f6dd1d5Computer Information
Benchmark Results
|
ext/DynamicPPLMCMCChainsExt.jl
Outdated
# Extract values from the chain | ||
values_dict = chain_sample_to_varname_dict(parameter_only_chain, sample_idx, chain_idx) | ||
# Resample any variables that are not present in `values_dict` | ||
_, varinfo = last( | ||
DynamicPPL.init!!( | ||
rng, | ||
model, | ||
varinfo, | ||
DynamicPPL.ParamsInit(values_dict, DynamicPPL.PriorInit()), | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that, if the chain does not store varnames inside its info
field, chain_sample_to_varname_dict
will fail.
I don't consider this to be a problem right now because every chain obtained via Turing's sample()
will contain varnames:
So this is only a problem if you manually construct a chain and try to call predict
on it, which I think is a highly unlikely workflow (and I'm happy to wait for people to complain if it fails). There are a few places in DynamicPPL's test suite where this does actually happen. I fixed them all by manually adding the varname dictionary.
However, it's obviously ugly. The only good way around this is, as I suggested before, to rework MCMCChains.jl. (See here for the implementation of the corresponding functionality in FlexiChains.)
init!!
to replace evaluate_and_sample!!
, predict
, returned
, and initialize_values
InitContext
, part 4 - Use init!!
to replace evaluate_and_sample!!
, predict
, returned
, and initialize_values
025aa8b
to
b55c1e1
Compare
b72c3bf
to
92d3542
Compare
7438b23
to
d55d378
Compare
12d93e5
to
7a8e7e3
Compare
1d8bceb
to
2edcd10
Compare
DynamicPPL.jl documentation for PR #984 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## py/init-prior-uniform #984 +/- ##
=========================================================
- Coverage 82.41% 80.57% -1.85%
=========================================================
Files 39 39
Lines 3992 3927 -65
=========================================================
- Hits 3290 3164 -126
- Misses 702 763 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4408efb
to
9c07727
Compare
5025592
to
3a16f9c
Compare
3a16f9c
to
4c96020
Compare
9c07727
to
ef038c6
Compare
4c96020
to
7b4c5fa
Compare
ef038c6
to
5961ca9
Compare
0656487
to
fd78d42
Compare
7b4c5fa
to
23cafe0
Compare
fd78d42
to
bfcdbb9
Compare
23cafe0
to
8587eb7
Compare
bfcdbb9
to
ab3e8da
Compare
8587eb7
to
a6a42bd
Compare
function tilde_assume(rng::Random.AbstractRNG, ::InitContext, sampler, right, vn, vi) | ||
@warn( | ||
"Encountered SamplingContext->InitContext. This method will be removed in the next PR.", | ||
) | ||
# just pretend the `InitContext` isn't there for now. | ||
return assume(rng, sampler, right, vn, vi) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was introduced to stop some test from failing (I think it was JETExt, but honestly, this PR was a month ago, so I already forgot exactly which one). It will indeed be removed together with SamplingContext.
""" | ||
predict([rng::Random.AbstractRNG,] model::Model, chain::AbstractVector{<:AbstractVarInfo}) | ||
Generate samples from the posterior predictive distribution by evaluating `model` at each set | ||
of parameter values provided in `chain`. The number of posterior predictive samples matches | ||
the length of `chain`. The returned `AbstractVarInfo`s will contain both the posterior parameter values | ||
and the predicted values. | ||
""" | ||
function predict( | ||
rng::Random.AbstractRNG, model::Model, chain::AbstractArray{<:AbstractVarInfo} | ||
) | ||
varinfo = DynamicPPL.VarInfo(model) | ||
return map(chain) do params_varinfo | ||
vi = deepcopy(varinfo) | ||
DynamicPPL.setval_and_resample!(vi, values_as(params_varinfo, NamedTuple)) | ||
model(rng, vi) | ||
return vi | ||
end | ||
end | ||
# Implemented & documented in DynamicPPLMCMCChainsExt | ||
function predict end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed at one of the meetings and we decided we didn't care enough about the predict
method on vectors of varinfos. It's currently bugged because varinfo
is always unlinked, but params_varinfo
might be linked, and if it is, it will give wrong results because it sets a linked value into an unlinked varinfo. See #983.
Part 1: Adding
hasvalue
andgetvalue
to AbstractPPLPart 2: Removing
hasvalue
andgetvalue
from DynamicPPLPart 3: Introducing
InitContext
andinit!!
This is part 4/N of #967.
In Part 3 we introduced
InitContext
. This PR makes use of the functionality in there to replace a bunch of code that no longer needs to exist:setval_and_resample!
followed by model evaluation: This process was used forpredict
andreturned
, to manually store certain values in the VarInfo, which would be used in the subsequent model evaluation. We can now do this in a single step usingParamsInit
.initialize_values!!
: very similar to the above. It would manually set values inside the varinfo, and then it would trigger an extra model evaluation to update the logp field. Again, this is directly replaced withParamsInit
.evaluate_and_sample!!
: direct one-to-one replacement withinit!!
.There is one API change associated with this: the
initial_params
kwarg tosample
must now be anAbstractInitStrategy
. It's still optional (it will usually default toPriorInit
). However, there are two implications:initial_params
cannot be a vector of parameters anymore. It must beParamsInit(::NamedTuple)
ORParamsInit(::AbstractDict{VarName})
.ParamsInit
expects values in unlinked space,initial_params
must always be specified in unlinked space. Previously,initial_params
would have to be specified in a way that matched the linking status of the underlying varinfo.I consider both of these to be a major win for clarity. (One might argue that vectors are more convenient. Sure, you can get a vector with
vi[:]
, but now you can just dovalues_as(vi, Dict{VarName,Any})
instead.)Closes
Closes #774
Closes #797
Closes #983
Closes TuringLang/Turing.jl#2476
Closes TuringLang/Turing.jl#1775